Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom label components. #95

Merged
merged 1 commit into from
Apr 12, 2018
Merged

Add support for custom label components. #95

merged 1 commit into from
Apr 12, 2018

Conversation

przywartya
Copy link
Contributor

No description provided.

Copy link
Contributor

@czosel czosel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks already really good! 👍

{{#if isLabel}}
<label class={{config.css.label}} for={{inputId}}>
{{label}}{{#if required}} {{requiredLabel}}{{/if}}
</label>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Indentation

<span>{{label}}</span>
<span style="display: inline-block;">
{{component (if labelComponent labelComponent "validated-label") label=label config=config inputId=inputId}}
</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would render a <label> inside another <label> - which is not exactly what we want here, right?
Maybe it would make sense to use this style of checkbox labels instead:

<input type="checkbox" id="coding" name="interest" value="coding">
<label for="coding">Coding</label>

(taken from MDN)

With this style, it seems that we could use labelComponent / validated-label exactly in the same way as with the other inputs. And it looks like all that needs to be changed is that the enclosing <label> tag has to be removed.

What do you think? 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Just realized that this is what isLabel is supposed to do. What do you think about using the label for="..." syntax instead, so that we don't need isLabel?

{{#if (or label labelComponent)}}
<span style="display: inline-block;">
{{component (if labelComponent labelComponent "validated-label")
isLabel=true label=label config=config inputId=inputId required=required}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer always using {{component labelComponent}} and just setting the default value of labelComponent to 'validated-label'

@przywartya
Copy link
Contributor Author

przywartya commented Apr 4, 2018

  • there was no required star for type checkbox input
  • there was no inputId for type checkbox input
    I have added both those things to checkbox in this PR. @anehx @czosel It seemed intentional that those weren't there and I'm confused 🤕
    @anehx I have added a default 👍
    @czosel {{inputId}} sometimes gives for="ember366-input-undefined" (when we don't satisfy name argument in {{validated-input}}). Tell me if that's okay for you? 😉

PS: Sorry for not replying so long (Easter holidays 🥚)

{{label}}{{#if required}} {{requiredLabel}}{{/if}}
</label>
{{#if (or label (not-eq labelComponent 'validated-label'))}}
<span style="display: inline-block;">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding a <span> here?

Copy link
Contributor Author

@przywartya przywartya Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: I think setting tagName: "span", on validated-label component is the cleanest way: is an inline element and doesn't destroy anything.

Ember is wrapping component render in <div id="ember..." class="ember-view"> </div>, but we want our label components to be inline with our input. So I have to add an inline-block style 😉

If you think thats ugly, then I can try this thing:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use tagName: 'label' on the validated-label component and remove it from the template? The for and class can be easily mapped too. This results in a cleaner DOM since we have way less elements..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I will do that!

@czosel
Copy link
Contributor

czosel commented Apr 9, 2018

Hi @przywartya,
sorry about the late reply! Code-wise, this looks great now. The only thing this is still missing is some tests - could you look into that? Let me know if I can help 😉

Oh, and would you mind also updating the README? ☺️

@przywartya
Copy link
Contributor Author

Updated :)

@czosel czosel merged commit 9a05441 into adfinis:master Apr 12, 2018
@czosel
Copy link
Contributor

czosel commented Apr 12, 2018

Looks great! Thanks for your awesome contribution 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants